Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactored parse_signature #61

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

kojix2
Copy link
Contributor

@kojix2 kojix2 commented Dec 23, 2020

Hi fiddle developers.

This is just an idea.
A proposal to improve the parse_signature method.

Background:
I wanted to parse_signature recursively when the argument was a function (i.e. a function pointer).
When I wrote a monkey patch for it, I noticed that parse_signature was unnecessarily complicated.

  1. Line 114 and line 117 are almost the same code.
  2. Each line is unnecessarily long.
  3. Some variable names are confusing.
  4. Also, the variables $1, $2, etc. looks old-fashioned.

2 - 4 are more of a library-wide issue, and are my personal opinions rather than objective facts.
Each library has its own history.

Here, I am focusing on 1.

If you think the pull request is not good, just close it.

Thank you.

@kojix2
Copy link
Contributor Author

kojix2 commented Dec 23, 2020

Hey, wait a minute. I think I made a mistake.

@Maumagnaguagno
Copy link
Contributor

You modified the regular expressions, from \* to *.
Also, you are calling an extra parse_ctype(ret, tymap) instead of just TYPE_VOIDP.
Probably better to keep such call inside the case statement: [parse_ctype($1.strip, tymap), $2, $3].

@kojix2
Copy link
Contributor Author

kojix2 commented Dec 23, 2020

Fixed.
@Maumagnaguagno I appreciate the help.

@Maumagnaguagno
Copy link
Contributor

I guess you need to propagate tymap to parse_ctype to avoid creating a new Hash in tymap ||= {}.

@kojix2
Copy link
Contributor Author

kojix2 commented Dec 23, 2020

Yes, I think you're right.
But this is the first time I've sent a PR to Fiddle.
I think I'll leave it as is and see what the project owner has to say about it...

Anyway, thank you again.

@kojix2
Copy link
Contributor Author

kojix2 commented Dec 23, 2020

I went back to work on the monkey patch one more time, but it wouldn't work without passing the tymap 🤦
I have to fix it. Maybe we should add a test for typealias...

@kou
Copy link
Member

kou commented Dec 23, 2020

I wanted to parse_signature recursively when the argument was a function (i.e. a function pointer).

Could you show a signature you want to parse?

@kou
Copy link
Member

kou commented Jan 5, 2021

An example: void uiSpinboxOnChanged(uiSpinbox *s, void (*f)(uiSpinbox *s, void *data), void *data)

@kojix2
Copy link
Contributor Author

kojix2 commented Jan 5, 2021

Yes, thanks.
I would like to create a customized method. When you pass a block to that method, it will create a Fidle::Closure::BlockCaller with the appropriate type. To do this, each function needs to know the type information beforehand.

@kou
Copy link
Member

kou commented Apr 12, 2021

Idea:

We use [TYPE, SIZE] for sized array type. e.g.: [Fiddle::TYPE_CHAR, 80] for char x[80]

It may be a good idea that we use [Fiddle::TYPE_VOIDP, RETURN_TYPE, [ARGUMENT_TYPE, ...]] for function pointer. e.g.: [Fiddle::TYPE_VOIDP, Fiddle::TYPE_VOID, [Fiddle::TYPE_VOIDP, Fiddle::Type_VOIDP]] for void (*f)(uiSpinbox *s, void *data)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants